test(utils): Add unit tests for HTTP utilities.#177
test(utils): Add unit tests for HTTP utilities.#177junhaoliao wants to merge 9 commits intoy-scope:mainfrom
Conversation
WalkthroughThis pull request introduces a strict coverage threshold for Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Unit Test
participant HttpUtils as HTTP Utility Module
participant Axios as Axios Library
Test->>HttpUtils: Call getUint8ArrayFrom (valid source)
HttpUtils->>Test: Return Uint8Array
Test->>HttpUtils: Call getUint8ArrayFrom (invalid source)
HttpUtils->>Test: Throw custom error or TypeError
Test->>HttpUtils: Call convertAxiosError with Axios error
HttpUtils->>Test: Return standardized error object
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This PR depends on #176 |
# Conflicts: # src/utils/http.ts
junhaoliao
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/utils/http.ts (1)
20-22: 🛠️ Refactor suggestionFollow coding guidelines for boolean expressions
As per coding guidelines, prefer
false == <expression>rather than!<expression>or0 === <expression>.- 0 === message.length ? + false == message.length ?
🧹 Nitpick comments (3)
src/utils/http.ts (1)
15-17: Consider using template literals for better readabilityThe string concatenation could be more readable using template literals.
- const details = "undefined" === typeof response ? - `${method} ${url} failed with no response: ${code}` : - `${method} returned ${response.status}(${response.statusText}): ${response.data as string}`; + const details = "undefined" === typeof response + ? `${method} ${url} failed with no response: ${code}` + : `${method} returned ${response.status}(${response.statusText}): ${response.data as string}`;test/utils/http.test.ts (2)
13-13: Consider using environment variable for endpointThe HTTP_BIN_ENDPOINT_BASE should be configurable via environment variables to support different testing environments.
-const HTTP_BIN_ENDPOINT_BASE = "https://cloud.yscope.com/httpbin"; +const HTTP_BIN_ENDPOINT_BASE = process.env.HTTP_BIN_ENDPOINT_BASE ?? "https://cloud.yscope.com/httpbin";
60-71: Improve error handling test patternThe try-catch pattern could be replaced with Jest's expect.rejects for better test readability.
- try { - // Access a 404 route. - await axios.get(url); - } catch (e) { - const error = convertAxiosError(e as AxiosError); - expect(error.cause).toEqual({ - url: url, - code: expectedCode, - details: `get returned ${expectedStatus}(${expectedStatusText}): ${expectedBody}`, - }); - } + await expect(axios.get(url)).rejects.toThrow(); + const error = convertAxiosError(await axios.get(url).catch(e => e)); + expect(error.cause).toEqual({ + url: url, + code: expectedCode, + details: `get returned ${expectedStatus}(${expectedStatusText}): ${expectedBody}`, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
jest.config.ts(1 hunks)package.json(1 hunks)src/utils/http.ts(1 hunks)test/utils/http.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,ts,tsx}`: - Prefer `false ==
**/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/utils/http.tsjest.config.tstest/utils/http.test.ts
**/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/utils/http.tsjest.config.tstest/utils/http.test.ts🔇 Additional comments (4)
src/utils/http.ts (1)
61-64: LGTM! Export statement is properly organizedThe export statement is well-structured and follows alphabetical ordering.
jest.config.ts (1)
40-44: LGTM! Coverage threshold aligns with other utilitiesThe coverage threshold for
src/utils/http.tsis appropriately set to 100% for branches, functions, and lines, matching the requirements for other utility files.test/utils/http.test.ts (2)
15-27: Add more test cases for getUint8ArrayFromThe current test suite only covers the happy path. Consider adding tests for:
- Large files
- Different content types
- Network timeouts
90-101: LGTM! Thorough error handling testThe test case for missing properties is well-designed and covers important edge cases.
…eract with the httpbin endpoint.
1 participant
Description
src/utils/http.ts.Checklist
breaking change.
Validation performed
npm run testand observed all tests succeeded, and there was no coverage violation.Summary by CodeRabbit
New Features
Chores
Tests